feat: calculate L1 TVL using batch request #37

0xnigir1 · 2024-08-02T16:15:21Z

🤖 Linear

Closes ZKS-75

Description

Retrieve L1 TVL of Shared Bridge from static token list:

l1Tvl method implemented
uses batchRequest for fetching token balances
uses Pricing Service for fetching prices

linear · 2024-08-02T16:15:23Z

ZKS-75 L1 TVL

Create a Batch contract to query token balances from SharedBridge

Get the top 30 list of token addresses and tickers by USD holdings on shared bridge from etherscan.

Measure how many token balance request can be made until fail or revert.
ex:

use 20 tokens
use 100 tokens
use 1000 tokens

and validate boundaries

BridgeHub address: 0x303a465B659cBB0ab36eE643eA362c509EEb5213

SharedBridge address: 0xD7f9f54194C633F36CCD5F3da84ad4a1c38cB2cB

AC:

Batch contract implemented
unit tests for batch contract
EvmProvider batchRequest() method used
Unit tests for L1MetricsService

0xkenj1

Have you tried out the functionality using a real world rpc node ?

I was thinking that we maybe want to add a README.md and or an example.ts with examples of metrics lib usage

i will write this down into linear tasks

0xkenj1 · 2024-08-02T19:45:33Z

libs/shared/src/utils/parseUnits.ts

@@ -0,0 +1,7 @@
+import BigNumber from "bignumber.js";
+
+export const parseUnits = (value: bigint, decimals: number): number => {


https://viem.sh/docs/utilities/parseUnits#parseunits is this the same ?

yeah you're right, i'll refactor to use: https://viem.sh/docs/utilities/formatUnits

0xkenj1 · 2024-08-02T19:48:48Z

libs/shared/src/tokens/tokens.ts

 export type TokenType = {
    name: string;
    symbol: string;
    coingeckoId: string;
    type: "erc20" | "native";


Suggested change

export type TokenType = {

name: string;

symbol: string;

coingeckoId: string;

type: "erc20" | "native";

export type TokenType<TokenType extends "erc20" | "native"> = {

name: string;

symbol: string;

coingeckoId: string;

type: TokenType;

0xkenj1 · 2024-08-02T19:49:32Z

libs/shared/src/tokens/tokens.ts

 export type TokenType = {
    name: string;
    symbol: string;
    coingeckoId: string;
    type: "erc20" | "native";
-    contractAddress: string | null;
+    contractAddress: Address | null;


Suggested change

contractAddress: Address | null;

contractAddress: TokenType == "erc20" ? Address : null;

0xkenj1 · 2024-08-02T19:49:53Z

libs/shared/src/tokens/tokens.ts

lmk if suggestions is what we want

0xkenj1 · 2024-08-02T19:53:18Z

libs/metrics/src/types/tvl.type.ts

+    amount: number;
+    amountUsd: number;
+    name: string;
+    imageUrl?: string;


should we add address here ?

0xkenj1 · 2024-08-02T19:55:16Z

libs/metrics/src/l1/l1MetricsService.ts

+
+        for (const token of tokens) {
+            const balance =
+                token.type === "native"


we can add 2 typeguards isNativeToken and isErc20Token

0xkenj1 · 2024-08-02T19:59:16Z

libs/metrics/src/l1/l1MetricsService.ts

+            .filter((token) => !!token.contractAddress)
+            .map((token) => token.contractAddress) as Address[];


0xkenj1 · 2024-08-02T20:05:23Z

libs/metrics/src/l1/l1MetricsService.ts

+        for (const token of tokens) {
+            const balance =
+                token.type === "native"
+                    ? balances[addresses.length]


is this correct ? always returning the same value for balance using address.length

Hmm I had the same doubt 🤔 but I think it's related with the structure of the return value of fetchTokenBalances. If I'm understanding correctly, the last element of that bigint[] array is a "special" value (which is being used here).

I'll write a suggestion at that method that might make things easier to understand here.

the custom contract for batch requests, requests token balances + native balanceOf(address) which in L1 is eth balance as the last element of balances array, so we always return addresses + 1 (since native token doesn't have a contract itself)

hmmmmmm, maybe using the batch contract just for tokens would be a better approach and would make the code more clear.

then you will have something like

{...calculateTvl(getTokenBalances()),getEthBalance()}

I would also rename TokenTvl to AssetTvl

Both calculateTvl(getTokenBalances()) and getEthBalance() would return AssetTvl

0xkenj1 · 2024-08-02T20:07:25Z

libs/metrics/src/l1/bytecode/index.ts

@@ -0,0 +1,2 @@
+export const tokenBalancesBytecode =


do we have a script to generate this? is it necessary to have it as variable? maybe just plain text is good enough, given that we are not getting any typing from this value.

for now, i manually copied the bytecode generated on contracts compilation (same as with the abi). i think adding a script that runs after compilation that copies the bytecode into the .ts file is too much at this stage but can be a future enhancement

regarding plain text, i didn't follow you here, what are you suggesting? the bytecode as string is needed to be passed as argument of batchRequest on EvmProvider method

i want to automate this for the cases in which the smart contract changes, so we use the compiled output each time we want to run or deploy or even running the pipeline.

this is kind of hardcoded, i don't like it if we have the contracts that can be compiled on the repo, maybe using the typechain version of viem could be an option.

i would have to write a script or copy and move the json with compilation info but i think the json has too much extra info that is not needed. however i think this script should be written on a separate PR

sounds good, i will add it to tech debt

0xyaco

Nice that you've introduced asserts 💯

0xyaco · 2024-08-02T21:25:30Z

libs/metrics/src/l1/l1MetricsService.ts

+            tokens.map((token) => token.coingeckoId),
+        );
+
+        assert(balances.length === addresses.length + 1, "Invalid balances length");


Out of curiosity, why is this assert necessary here with the + 1? If it's related to the natspec for the fetchTokenBalances method, this assert might be better located inside the method?

0xyaco · 2024-08-02T21:57:16Z

libs/metrics/src/l1/l1MetricsService.ts

+        for (const token of tokens) {
+            const balance =
+                token.type === "native"
+                    ? balances[addresses.length]


Hmm I had the same doubt 🤔 but I think it's related with the structure of the return value of fetchTokenBalances. If I'm understanding correctly, the last element of that bigint[] array is a "special" value (which is being used here).

I'll write a suggestion at that method that might make things easier to understand here.

0xyaco · 2024-08-02T22:10:38Z

libs/metrics/src/l1/l1MetricsService.ts

+            returnAbiParams,
+        );
+
+        return balances as bigint[];


I'm suggesting this purely from a pure code perspective, I've got no idea about the bussiness logic behind this so feel free to tell me that I'm absolutely wrong lol

Trying to make things a little bit more declarative, the fetchTokenBalances could return some kind of object that distinguishes between the eth balance and the rest of the balances as it seems that the eth balance is a pretty particular case for the consumers of this data in your code:

Suggested change

return balances as bigint[];

return { ethBalance: balances[balances.length], addressesBalances: balances.slice(0, -1) }

Something like that might make things easier to use: you can change the assertion I've commented on by dropping the + 1 that feels strange at first sight and also, there will be no "obscure" accesses to balances[balances.length] in the future, as it might bring some confusion to the reader.

this approach looks quite better and its better than the one that i've pointed out in my prev comment.

0xyaco · 2024-08-02T22:17:17Z

libs/metrics/test/unit/l1/l1MetricsService.spec.ts

@@ -78,9 +118,77 @@ describe("L1MetricsService", () => {
    });

    describe("l1Tvl", () => {
-        it("return l1Tvl", async () => {
+        it("return the TVL on L1 Shared Bridge", async () => {
+            const mockBalances = [60_841_657_140641n, 135_63005559n, 12_3803_824374847279970609n]; // Mocked balances


Any special reason to have 135_63005559n instead of 13_563_005_559n?

this is to now where the decimal part starts on the bigint 😅 , do you think this is misleading?

0xkenj1 · 2024-08-05T17:34:17Z

libs/metrics/src/l1/l1MetricsService.ts

+        for (const token of tokens) {
+            const balance =
+                token.type === "native"
+                    ? balances[addresses.length]


hmmmmmm, maybe using the batch contract just for tokens would be a better approach and would make the code more clear.

then you will have something like

{...calculateTvl(getTokenBalances()),getEthBalance()}

I would also rename TokenTvl to AssetTvl

Both calculateTvl(getTokenBalances()) and getEthBalance() would return AssetTvl

0xkenj1 · 2024-08-05T17:37:28Z

libs/metrics/src/l1/l1MetricsService.ts

+            returnAbiParams,
+        );
+
+        return balances as bigint[];


this approach looks quite better and its better than the one that i've pointed out in my prev comment.

0xnigir1 · 2024-08-06T00:18:03Z

I've refactored taking into account your suggestions and code is definitely cleaner now 🤝
Also, I've changed the return type of the method to be an AssetTvl[] since they have to be ordered descending by TVL. Also this will make easier to add pagination if required in the future

0xyaco · 2024-08-06T15:21:53Z

libs/metrics/src/l1/l1MetricsService.ts

+            const balance = isNativeToken(token)
+                ? balances.ethBalance
+                : balances.addressesBalance[
+                      addresses.indexOf(tokenInfo.contractAddress as Address)
+                  ];


0xyaco · 2024-08-06T15:45:39Z

libs/metrics/src/l1/l1MetricsService.ts

        }

+        // we assume the rounding error is negligible for sorting purposes
+        tvl.sort((a, b) => Number(b.amountUsd) - Number(a.amountUsd));


Good clarifying comment there 💯 , just one extremely nitpick question but figured is worth asking: this might cause tvl to have elements with equal amountUsd to be sorted differently even if run with the same set of data [1]. Are we ok with that?

Eg, this is potentially possible:

> tvl.sort() [{name: "A", amountUsd: 1}, {name: "B", amountUsd: 2}] > tvl.sort() [{name: "B", amountUsd: 1}, {name: "A", amountUsd: 2}]

It seems that the JavaScript engines now generally tend to implement the sort function in a stable way, but there's a second thing to have in mind also: the order of the returned tvl, if the sort is stable, will probably depend on the order of the input tokens.

To wrap up, is the order of tvl critical? If yes, you might also sort by name (when amountUsd values are equal) or something like that to always have a consistent order, without depending on the sort implementation or the tokens order. If this is not the case, this works perfectly. 💯

[1] https://www.30secondsofcode.org/js/s/array-stable-sort/#:~:text=The%20ECMAScript%20specification%20does%20not,to%20be%20preserved%20after%20sorting.

Hmm, this is something we might not need to care about for now, but it's a good observation. Let's keep it as it is for now.

i think that the chances of two tokens to have the same tvl is really low on the most relevant tokens, this is more likely the situation on token with low balances or 0ish TVL
adding a second sort by name i think is not necessary for now (we don't care if PEPE goes before or after MAGAIBA xd)

hahahahaha , nigiri is right. However we should be aware of this if we need to change it in the future. :)

0xkenj1 · 2024-08-06T17:40:12Z

libs/metrics/src/l1/bytecode/index.ts

@@ -0,0 +1,2 @@
+export const tokenBalancesBytecode =


sounds good, i will add it to tech debt

feat: calculate L1 TVL using batch request

539530f

0xnigir1 requested review from 0xkenj1 and 0xyaco August 2, 2024 16:15

0xkenj1 requested changes Aug 2, 2024

View reviewed changes

0xyaco reviewed Aug 2, 2024

View reviewed changes

fix: use formatUnits from viem & fix visual test

6747ea4

0xkenj1 requested changes Aug 5, 2024

View reviewed changes

fix: make code cleaner & readable, use bigint for math

0d51d95

0xnigir1 requested review from 0xyaco and 0xkenj1 August 6, 2024 00:18

0xyaco reviewed Aug 6, 2024

View reviewed changes

0xkenj1 approved these changes Aug 6, 2024

View reviewed changes

0xyaco approved these changes Aug 6, 2024

View reviewed changes

0xnigir1 merged commit 4ef73d9 into dev Aug 6, 2024
6 checks passed

0xnigir1 deleted the feat/l1-tvl-impl branch August 6, 2024 19:33

		@@ -0,0 +1,7 @@
		import BigNumber from "bignumber.js";

		export const parseUnits = (value: bigint, decimals: number): number => {

	contractAddress: Address \| null;
	contractAddress: TokenType == "erc20" ? Address : null;

		.filter((token) => !!token.contractAddress)
		.map((token) => token.contractAddress) as Address[];

	return balances as bigint[];
	return { ethBalance: balances[balances.length], addressesBalances: balances.slice(0, -1) }

feat: calculate L1 TVL using batch request #37

feat: calculate L1 TVL using batch request #37

Conversation

0xnigir1 commented Aug 2, 2024

🤖 Linear

Description

linear bot commented Aug 2, 2024

0xkenj1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xkenj1 Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xyaco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xnigir1 commented Aug 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xkenj1 Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xkenj1 Aug 2, 2024 •

edited

Loading

0xkenj1 Aug 6, 2024 •

edited

Loading